-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add Gemini CLI integration using @google/gemini-cli-core #7354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add GeminiCliHandler provider for OAuth-based Gemini access - Add gemini-cli type definitions and model configurations - Update provider registry to include gemini-cli - Add comprehensive tests for GeminiCliHandler - Add @google/gemini-cli-core as dependency (pending npm package) This integration allows users to authenticate with Gemini using the official Gemini CLI OAuth flow instead of API keys, providing a more secure and sustainable authentication method. Addresses #7353
| }) | ||
| }) | ||
|
|
||
| describe("constructor", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests use setTimeout (150ms) to wait for async initialization. Consider refactoring to await an explicit initialization promise if available; this would improve test reliability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this code 5 minutes ago and I'm already confused by my own async initialization pattern.
|
|
||
| // Dynamically import the Gemini CLI library | ||
| // This allows the code to compile even if the package isn't installed yet | ||
| this.initializeClient(oauthPath, projectId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? The async initialization in the constructor creates a race condition. Since constructors can't be async, calling initializeClient() without awaiting means the client might not be initialized when methods are called immediately after instantiation.
| this.initializeClient(oauthPath, projectId) | |
| constructor(options: ApiHandlerOptions) { | |
| super() | |
| this.options = options | |
| // Store initialization promise to await in methods | |
| const oauthPath = this.options.geminiCliOAuthPath || "~/.config/gemini-cli/oauth.json" | |
| const projectId = this.options.geminiCliProjectId | |
| this.initPromise = this.initializeClient(oauthPath, projectId) | |
| } |
Then await this.initPromise in each method instead of the unreliable 100ms timeout.
| private async ensureClientInitialized() { | ||
| if (!this.client) { | ||
| // Wait a bit for initialization to complete | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 100ms timeout here is not a reliable solution for async initialization. Could we use a more robust pattern like storing the initialization promise and awaiting it?
| await new Promise((resolve) => setTimeout(resolve, 100)) | |
| private async ensureClientInitialized() { | |
| if (this.initPromise) { | |
| await this.initPromise | |
| } | |
| if (!this.client) { | |
| throw new Error("Gemini CLI client not initialized. Please check your configuration.") | |
| } | |
| } |
| */ | ||
| export class GeminiCliHandler extends BaseProvider implements SingleCompletionHandler { | ||
| protected options: ApiHandlerOptions | ||
| private client: any // Will be typed when @google/gemini-cli-core is available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using any type reduces type safety. Could we create a proper type definition for the expected client structure, even if it's just a minimal interface?
| private client: any // Will be typed when @google/gemini-cli-core is available | |
| private client: GeminiCliClient | undefined // Define GeminiCliClient interface |
|
|
||
| // Initialize the Gemini CLI client with OAuth configuration | ||
| // The OAuth path and project ID come from the provider settings | ||
| const oauthPath = this.options.geminiCliOAuthPath || "~/.config/gemini-cli/oauth.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tilde expansion might not work correctly in all environments. Should we use proper home directory resolution?
| const oauthPath = this.options.geminiCliOAuthPath || "~/.config/gemini-cli/oauth.json" | |
| const oauthPath = this.options.geminiCliOAuthPath || path.join(os.homedir(), '.config/gemini-cli/oauth.json') |
Don't forget to import path and os modules.
| projectId, | ||
| }) | ||
| } catch (error) { | ||
| throw new Error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error won't be caught properly since it's thrown in an async function that's not awaited in the constructor. Consider handling this differently or documenting that initialization errors will result in unhandled promise rejections.
| }) | ||
| } catch (error) { | ||
| throw new Error( | ||
| "@google/gemini-cli-core is not installed. Please install it with: npm install @google/gemini-cli-core", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other error messages in this file, should this also use i18n?
| "@google/gemini-cli-core is not installed. Please install it with: npm install @google/gemini-cli-core", | |
| throw new Error(t("common:errors.gemini.cli_not_installed")) |
|
|
||
| export const geminiCliDefaultModelId: GeminiCliModelId = "gemini-2.0-flash-001" | ||
|
|
||
| // Re-use the same model definitions as regular Gemini since they're the same models |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good approach reusing the existing Gemini model definitions! This avoids duplication and ensures consistency. Just noting that when the package becomes available, we might want to verify these models are actually supported through the CLI.
| }) | ||
| }) | ||
|
|
||
| describe("constructor", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a test for concurrent initialization - what happens if multiple methods are called simultaneously before initialization completes? This could help validate the race condition handling.
Summary
This PR attempts to address Issue #7353 by implementing official Gemini CLI integration using the @google/gemini-cli-core library. Feedback and guidance are welcome.
Changes
Implementation Details
This integration provides a library-based approach to accessing Gemini models through OAuth authentication via the official Gemini CLI, as requested in the issue. The implementation:
Testing
Next Steps
Once the @google/gemini-cli-core package is published to npm, users will be able to:
Related Issues
Closes #7353
Checklist
Important
Adds Gemini CLI integration using
@google/gemini-cli-corewith OAuth support, comprehensive tests, and tiered pricing cost calculation.GeminiCliHandlerfor OAuth-based Gemini access using@google/gemini-cli-core.buildApiHandler()inindex.tsto includegemini-cliprovider.@google/gemini-cli-corepackage with dynamic imports and error messages.gemini-cli.ts.gemini-cli.ts.GeminiCliHandleringemini-cli.spec.ts.@google/gemini-cli-coretopackage.jsondependencies.This description was created by
for 2df8103. You can customize this summary. It will automatically update as commits are pushed.